fix(constant-time-analysis): restore dis offset column on Python 3.13+ - #200
fix(constant-time-analysis): restore dis offset column on Python 3.13+#200farhan6667 wants to merge 2 commits into
Conversation
Python 3.13 stopped printing the bytecode byte-offset column in `python -m dis` output by default (see What's New in Python 3.13), adding `-O`/`--show-offsets` to opt back in. PythonAnalyzer's _parse_dis_output regex requires that offset column, so on 3.13+ every instruction line silently failed to match: total_instructions stayed at 0 and the analyzer reported "PASSED, no violations" on code with genuine timing side-channels (e.g. an early-exit byte-by-byte comparison on secret data). _get_dis_output now detects --show-offsets support at runtime and passes it only when available, so it keeps working unmodified on Python < 3.13 where the flag doesn't exist. Verified against the bundled tests/test_samples/vulnerable.py sample: before this change it reported 0 instructions analyzed and "PASSED"; after, it correctly reports 328 instructions and multiple violations. Added a regression test that would have caught this.
|
|
|
@farhan6667 — this is the highest-value PR in the queue right now and I want to say so clearly, because it's been sitting. I reproduced the bug independently before writing this. r"(?:(\d+)\s+)?(\d+)\s+([A-Z_]+)\s*(.*)"which requires a numeric offset column. Python 3.13 stopped emitting that column from Mid-expression operations are exactly where variable-time arithmetic lives, so on 3.13+ the analyzer discards the dangerous instructions and reports PASSED. A constant-time checker that silently passes everything is the worst failure mode this plugin has. Two things block merging, and both need you:
Do those two and I'll get it reviewed and merged promptly. Thanks for catching this. |
Summary
PythonAnalyzerinplugins/constant-time-analysis/ct_analyzer/script_analyzers.pycallspython -m dis <file>and parses the output with a regex that requires a numeric byte-offset column before each instruction mnemonic.Python 3.13 stopped printing that offset column by default (see What's New in Python 3.13) and added
-O/--show-offsetsto opt back in. Without that flag on 3.13+, every instruction line in_parse_dis_outputsilently fails to match:total_instructionsstays0and the analyzer reports "PASSED, no violations" on Python code with genuine timing side-channels.Reproduced with the plugin's own bundled sample:
The same is true for a minimal early-exit secret comparison (the canonical example this tool exists to catch):
Fix
_get_dis_outputnow detects whether--show-offsetsis supported (viapython -m dis --help) and passes it only when available, so behavior on Python < 3.13 (which doesn't have the flag) is unchanged.After the fix, on Python 3.13:
Testing
test_get_dis_output_includes_offsets_on_py313_plus, a regression test that fails without this fix on Python 3.13+ (skips itself on older Python where--show-offsetsdoesn't exist).python3 -m pytest ct_analyzer/tests/test_analyzer.py— 59 passed, 1 pre-existing failure unrelated to this change (test_cross_compile_arm64fails in my sandbox due to missing local glibc headers for arm64 cross-compilation, not related to the Python analyzer).ruff checkandruff format --diffboth clean on the two changed files.Note on the existing integration test
test_python_vulnerable_detectedalready exercises this exact path but its assertion isif report.error_count > 0: ..., which passes vacuously whenerror_countstays0— that's how this false negative shipped without CI catching it. I left that test as-is to keep this PR focused on the root-cause fix plus one new regression test that would actually have caught it; happy to also tighten that assertion in this PR or a follow-up if maintainers prefer.